Repository navigation
Conversation
…nto new branches
|
@claude /review and report only on actionable issues. |
|
Claude finished @bctiemann's task in 2m 17s —— View job Review
The fix is correct and the approach is sound. One actionable issue and one minor note. Actionable: M2M through tables of resolver-opted-in models are not replicated
For example, Fix: after adding if (
meta.db_table not in tables
and meta.db_table in existing_tables
and meta.managed
and not meta.proxy
and not issubclass(model, ChangeLoggingMixin)
and supports_branching(model)
):
tables.add(meta.db_table)
for m2m_field in meta.local_many_to_many:
if hasattr(m2m_field, 'through'):
tables.add(m2m_field.through._meta.db_table)
else:
tables.add(m2m_field._get_m2m_db_table(meta))Minor:
|
…anches M2M writes follow the instance's database, so without the branch's own copy they fell through to main's through table. Also drop a redundant schema cleanup from the branch test; the test case's teardown already drops it.
|
Related, for the same release: #732 adds the missing-table check this PR has on its new loop to the existing object-type path of |
arthanson
left a comment
arthanson
left a comment
There was a problem hiding this comment.
Just the one nit about imports, otherwise - nice work!
| # A model that a plugin's resolver makes branchable without change logging has no branching | ||
| # feature, but the router sends its queries to the branch, so the branch needs its table too. | ||
| if _branching_resolvers: | ||
| from django.apps import apps |
There was a problem hiding this comment.
these are being re-imported for no reason - I think the ChangeLoggingMixin is the only one that needs to be here
Fixes: #728
The router sends the queries of a model that a plugin's resolver makes branchable to the branch, but provisioning copied only the tables of object types with the
branchingfeature, plus their M2M through tables. A resolver-only model has no such object type, so its table wasn't copied, and its queries fell through to main's table.Changes:
get_tables_to_replicate()also includes the table of each registered model that isn't change-logged and thatsupports_branching()says is branchable, plus its M2M through tables, when any resolver is registered. Proxy, unmanaged and missing tables are skipped, andexempt_modelsstill applies throughsupports_branching().docs/plugin-development.md: a resolver returningTruealso gives new branches their own copy of the model's table and its M2M through tables.Tests:
auth.Group, its table andauth_group_permissionsare inget_tables_to_replicate(), and aren't when the model is exempt.auth_group, and renaming a group or adding a permission to it in the branch leaves main's unchanged.main.Branches provisioned before this change still have no copy of these tables. That's the same situation as #683.